-
Notifications
You must be signed in to change notification settings - Fork 60
iptunnel: add support to ipip, ipip6 and ip6ip6 tunnels #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
liangwen12year
commented
May 19, 2025
669f960
to
24fa311
Compare
)?, | ||
))) | ||
} | ||
_ => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no default in kernel for parsing this:
net/ipv4/ip_tunnel_core.c
1158: if (data[IFLA_IPTUN_FLAGS]) {
1161: flags = nla_get_be16(data[IFLA_IPTUN_FLAGS]);
net/ipv6/ip6_tunnel.c
1992: if (data[IFLA_IPTUN_FLAGS])
1993: parms->flags = nla_get_u32(data[IFLA_IPTUN_FLAGS]);
2104: /* IFLA_IPTUN_FLAGS */
2134: nla_put_u32(skb, IFLA_IPTUN_FLAGS, parm->flags) ||
2170: [IFLA_IPTUN_FLAGS] = { .type = NLA_U32 },
net/ipv6/sit.c
1668: /* IFLA_IPTUN_FLAGS */
1708: nla_put_be16(skb, IFLA_IPTUN_FLAGS,
1748: [IFLA_IPTUN_FLAGS] = { .type = NLA_U16 },
Always do explicit kind matching and fail for unknown kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the explicit kind matching and fail for an unknown kind.
src/link/link_info/iptunnel.rs
Outdated
Flags(TunnelFlags::from_bits_retain(u32::from( | ||
parse_u16(payload).context( | ||
"invalid IFLA_IPTUN_FLAGS value for SIT", | ||
)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of storing u16 into u32.
I prefer InfoIpTunnel::Ipv6SitFlags(u16)
, InfoIpTunnel::Ipv6Flags(u32)
, InfoIpTunnel::Ipv4Flags(u16)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added InfoIpTunnel::Ipv6SitFlags(u16), InfoIpTunnel::Ipv6Flags(u32), InfoIpTunnel::Ipv4Flags(u16)
as you suggested.
src/link/link_info/iptunnel.rs
Outdated
| Ipv6RdRelayPrefixLen(value) => { | ||
NativeEndian::write_u16(buffer, *value) | ||
} | ||
Protocol(value) => buffer[0] = i32::from(*value) as u8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The as
is dangerous keyword because it discard data silently.
According to https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml , it is always u8.
So implement From<u8> for IpProtocol
there instead of using as
. When you do that, please fix src/rule/attribute.rs
also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented From<u8> for IpProtocol
instead of using as
. I also fixed src/rule/attribute.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also include unit test for ipv6 site support.
All these tunnels use the IFLA_IPTUN_* netlink API. Therefore, both IFLAN_INFO_KIND "ipip" and "ip6tnl" data is serialized using the IpTunnel struct. Unit tests added. Signed-off-by: Fernando Fernandez Mancera <[email protected]>
According to the IANA protocol number specification (https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml), protocol numbers are always within the u8 range. Using `as` for type conversion is dangerous because it can silently discard higher-order data, leading to bugs that are hard to detect. This change ensures all protocol number conversions are type-safe and explicit, avoiding accidental data loss and aligning the code with the protocol specification.
a75e419
to
cbb7c22
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 67.41% 67.83% +0.42%
==========================================
Files 140 142 +2
Lines 9508 9925 +417
==========================================
+ Hits 6410 6733 +323
- Misses 3098 3192 +94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cbb7c22
to
e2a2c44
Compare
Thanks, I added the unit test for the sit support. |
@@ -188,7 +189,8 @@ pub enum InfoKind { | |||
MacVtap, | |||
GreTap, | |||
GreTap6, | |||
IpTun, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't this breaks retrocompatibility ? maybe we need to transform IpTun into an alis to IpIp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok to break API before we reach 1.0.
This IpTun
is clearly wrong because kernel use ipip
string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
const IFLA_IPTUN_LINK: u16 = 1; | ||
const IFLA_IPTUN_LOCAL: u16 = 2; | ||
const IFLA_IPTUN_REMOTE: u16 = 3; | ||
const IFLA_IPTUN_TTL: u16 = 4; | ||
const IFLA_IPTUN_TOS: u16 = 5; | ||
const IFLA_IPTUN_ENCAP_LIMIT: u16 = 6; | ||
const IFLA_IPTUN_FLOWINFO: u16 = 7; | ||
const IFLA_IPTUN_FLAGS: u16 = 8; | ||
const IFLA_IPTUN_PROTO: u16 = 9; | ||
const IFLA_IPTUN_PMTUDISC: u16 = 10; | ||
const IFLA_IPTUN_6RD_PREFIX: u16 = 11; | ||
const IFLA_IPTUN_6RD_RELAY_PREFIX: u16 = 12; | ||
const IFLA_IPTUN_6RD_PREFIXLEN: u16 = 13; | ||
const IFLA_IPTUN_6RD_RELAY_PREFIXLEN: u16 = 14; | ||
const IFLA_IPTUN_ENCAP_TYPE: u16 = 15; | ||
const IFLA_IPTUN_ENCAP_FLAGS: u16 = 16; | ||
const IFLA_IPTUN_ENCAP_SPORT: u16 = 17; | ||
const IFLA_IPTUN_ENCAP_DPORT: u16 = 18; | ||
const IFLA_IPTUN_COLLECT_METADATA: u16 = 19; | ||
const IFLA_IPTUN_FWMARK: u16 = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an enum is better here ?
#[repr(u16)]
enum IPTunKind {
Link
Local
Remove
}
The values are consecutive calculated by rust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repr(u16)
will not works for Other(u16)
for as
transmute. And using repr
forbid rust to find smaller datatype for this enum. So using repr
brings no benefit.
src/link/link_info/iptunnel.rs
Outdated
if payload.len() == 4 { | ||
let mut data = [0u8; 4]; | ||
data.copy_from_slice(&payload[0..4]); | ||
Self::Local(IpAddr::V4(Ipv4Addr::from(data))) | ||
} else if payload.len() == 16 { | ||
let mut data = [0u8; 16]; | ||
data.copy_from_slice(&payload[0..16]); | ||
Self::Local(IpAddr::V6(Ipv6Addr::from(data))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this pattern quite repeated like, ipv4 vs ipv6 can you factor that out ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have parse_ip_addr()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then let's use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used parse_ip_addr()
instead, thanks!
pub enum TunnelEncapType { | ||
None = TUNNEL_ENCAP_NONE, | ||
Fou = TUNNEL_ENCAP_FOU, | ||
Gue = TUNNEL_ENCAP_GUE, | ||
Mpls = TUNNEL_ENCAP_MPLS, | ||
Other(u16), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the constants here, I think you can directly use TunnelEncapType::[Kind] everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Other thing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we need constants in the Rust code to provide an explicit and stable mapping between each enum variant and its corresponding protocol value as defined by the kernel. Using constants such as TUNNEL_ENCAP_FOU or TUNNEL_ENCAP_GUE ensures that the integer values we use in serialization and deserialization always match the values expected by the kernel, regardless of the order of enum variants in Rust or any future refactoring.
This improves the maintainability and clarity of the code by making protocol mappings immediately clear to anyone reading the code.
impl From<u16> for TunnelEncapType { | ||
fn from(d: u16) -> Self { | ||
match d { | ||
TUNNEL_ENCAP_NONE => Self::None, | ||
TUNNEL_ENCAP_FOU => Self::Fou, | ||
TUNNEL_ENCAP_GUE => Self::Gue, | ||
TUNNEL_ENCAP_MPLS => Self::Mpls, | ||
_ => Self::Other(d), | ||
} | ||
} | ||
} | ||
|
||
impl From<TunnelEncapType> for u16 { | ||
fn from(d: TunnelEncapType) -> Self { | ||
match d { | ||
TunnelEncapType::None => TUNNEL_ENCAP_NONE, | ||
TunnelEncapType::Fou => TUNNEL_ENCAP_FOU, | ||
TunnelEncapType::Gue => TUNNEL_ENCAP_GUE, | ||
TunnelEncapType::Mpls => TUNNEL_ENCAP_MPLS, | ||
TunnelEncapType::Other(value) => value, | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really needed if we just use the const with the reprt everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[repr]
should not be used as the as
action will not works for enum with Other(u32)
variant.
e2a2c44
to
0f97da2
Compare
0f97da2
to
5b9a3a8
Compare